Say, DN is healthy, all containers confirmed, no new allocations → that DN's bucket never rolls even though heartbeats come every 30 seconds, right?

t=0    Container C1 allocated → pending recorded in tracker

t=60-120  FCR arrives from DN
          → cid = C1
          → alreadyInDn = expectedContainersInDatanode.remove(C1) = FALSE
          → !alreadyInDn = TRUE → removePendingAllocation called → rollIfNeeded fires ✓
          → C1 added to NM DN-set

How abt rolls on every processHeartbeat, every 30 seconds regardless of container state changes ?

Potentially hits concurrency issue. Say two threads entered this block.

Thread-1 (removePendingAllocation): bucket.isEmpty(), returns true

Thread-2 (recordPendingAllocationForDatanode): computeIfAbsent(uuid) returns same bucket
reference (key still exists), calls bucket.add(containerID) and now the bucket will be non-empty

Thread-1: datanodeBuckets.remove(uuid, bucket), then removes the non-empty bucket and now the containerID will be in a detached bucket object, right?

I think, we need to add synchronization to avoid detached bucket object.

Do we have test scenario covering roll over?

The two-window rolling behavior (container in previousWindow roll after 2× interval). Say, add C1 in currentWindow, then moves C1 to previousWindow, then wait for the roll over.

we can have config as hdds.scm.container.pending.allocation.roll.interval

This needs to be part of SCMNodeManager, more specific to SCMNodeStat. Reason,

1. need handle even like stale node / dead node handler as cleanup
2. May need report this when reporting to CLI for available space in the DN
3. To be used for pipeline allocation policy, where container manager does not come in role
   Its datanode space, just trying to identify already allocated space. And needs to be part of committed space at SCM when reporting to CLI, or other breakup.

Do we really need check if container exist? or just remove if exist as single call ?

Agreed. Instead of checking and then removing. We can just check the pending list and remove it. It will be the same, We can just avoid one extra op.

Just checked, This inherently does a copy of the containers with a TreeSetgetExisting(id).copyContainers();, We should avoid this at all cost.

Please check if node report is also send in ICR, this is for reason that node information should be updated with ICR at same time.

Instead of calcuating all available and then removing, we can do progressive base, like,
required=pending+newAllocation
for each report
required = required - volumeUsage in roundoff value
if (required <= 0)
return true

But we need to reserve also, can do first add and check, if not present, remove containerId

OR other way,
when DN report storage handling, total consolidate value can also be added to memory to avoid looping on every call.

This is costly operation as first it combine 2 list, and then it performs count.

if we use synchronized in all methods, then set need not be threadsafe.

rolling period is 5 min or 10 min ? mean previous bucket to be there for additional 5 min to capture containerList IN-Progress

code handling different from ICR and FCR, can be same only.

If we are not sure if we can create container here, Should we still choose this pipeline? Instead of making it generic, we can specify what to do for each exception we might see.

Why not just add this to the ContainerManager interface? We can avoid these conversions. Is this because Recon uses the same code path and we don't want it to this? For Recon we can just make it a No-Op.

Agreed. Instead of checking and then removing. We can just check the pending list and remove it. It will be the same, We can just avoid one extra op.

Nit: We can import DatanodeInfo

Can we just return the count of both windows instead of adding them to separate set just to get the count here.

Pending allocations can persist beyond 2× roll interval after long idle periods because rollIfNeeded() only rolls once.A single roll doesn’t clear entries older than two windows which can incorrectly block new allocations.

This makes the allocation little aggressive right? Even if we just have 5GB we allocate it. Should we have leave some buffer when allocating a container?

We can change all the LOG to debug. This will become too noisy on the SCM log for every allocation and removal.

Same as above for all the LOG in this class.